Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix TransformAt for Polyline #1035

Merged

Conversation

DmytroMuravskyi
Copy link
Contributor

@DmytroMuravskyi DmytroMuravskyi commented Oct 4, 2023

BACKGROUND:

DESCRIPTION:

  • The issue is tracked down to Polyline.TransformAt where parameter value is treated as domain value in one place but still as 0 to 1 in other place. The function is updated.
  • It also treated TransformAt incorrectly for first and last vertex in the list if Polyline is actually a Polygon. Fixed it so all transforms at vertices are averaged.
  • I also found that IndexedPolycurve.CreateVerticesAndCurveIndices produced unique indexes. For two lines and arc in between it would be [0, 1], [2, 3, 4], [5, 6]. It contradicted with the fact that vertices in the same function were considered as shared but also changes indices when polyline is copies. If the TransformedPolycurve is called on polycurve with indices [0, 1], [1, 2, 3], [3, 4], new polycurve will have indices as described above: [0, 1], [2, 3, 4], [5, 6] indices that out of range of verticies. Did the fix.

TESTING:

  • Added PreservesIndicesTransformed and PolylineTransformAt tests.

FUTURE WORK:

  • There is discussion about introducing a convenient way to get Points from polyline uniformly distributed by length, since PointAtNormalized is not doing the job.

REQUIRED:

  • All changes are up to date in CHANGELOG.md.

COMMENTS:

  • Any other notes.

This change is Reviewable

@ikeough
Copy link
Contributor

ikeough commented Oct 11, 2023

Thanks @DmytroMuravskyi for continuing to dig us out after the curve changes :)

# Conflicts:
#	CHANGELOG.md
#	Elements/test/IndexedPolyCurveTests.cs
#	Elements/test/PolylineTests.cs
@DmytroMuravskyi
Copy link
Contributor Author

Updated branch and solved conflicts

@wynged wynged requested review from andrewheumann and ikeough and removed request for andrewheumann October 25, 2023 14:00
Copy link
Member

@andrewheumann andrewheumann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 5 of 5 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status: :shipit: complete! 1 of 1 approvals obtained (waiting on @ikeough)

@andrewheumann andrewheumann merged commit 8900409 into hypar-io:master Oct 31, 2023
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants